route - #1753
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
f5188c3 to
75fbc59
Compare
20dc819 to
c54556c
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
c54556c to
e4d6dfb
Compare
75fbc59 to
8086c9c
Compare
e4d6dfb to
ddd5684
Compare
dd9b98e to
83d440b
Compare
ddd5684 to
597e7a7
Compare
83d440b to
bbcc339
Compare
597e7a7 to
7fba952
Compare
bbcc339 to
73933c6
Compare
7fba952 to
a1ca24d
Compare
73933c6 to
493afc8
Compare
a1ca24d to
6a68c58
Compare
8bd7e1c to
e7119dc
Compare
daniel-noland
added
bug
4275bcc to
364a49e
Compare
98d9cda to
b760312
Compare
b5c811d to
27dde18
Compare
dd860c4 to
8bf6ba3
Compare
0831e03 to
99bedd4
Compare
8bf6ba3 to
4262cba
Compare
99bedd4 to
89503aa
Compare
4262cba to
cfaa23b
Compare
89503aa to
566baca
Compare
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Use changelog to model check fib. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
A fib may be indexed by both id and vni. Make sure to delete every entry referring to the target fib so no alias outlives its writer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
- base receive framing on used (rather than the resized) buffer length. - Wait for complete headers, - handle partial bodies, - reject announced bodies over 16 MiB, Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
An empty-next-hop route in the rib was rejected by the fib. This allowed traffic to fall through to a less-specific route. We now substitute an explicit drop so every caller preserves consistency. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
- generate VRF status transitions - distinguish preset root-drop routes from ordinary drop routes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Interface names come from the kernel. Letting these names say in the key can give the same next-hop different keys. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Daniel Noland <daniel@githedgehog.com>
cfaa23b to
18802fb
Compare
566baca to
bb22af3
Compare
| #[cfg(test)] | ||
| fn quick_resolve_rec(&self, result: &mut BTreeSet<NhopKey>) { | ||
| fn quick_resolve_rec(&self, result: &mut BTreeSet<NhopKey>, visited: &mut Visited) { | ||
| if visited.contains(&self.id()) { |
There was a problem hiding this comment.
No need to fix but for really deep (say 1k+ routes) the performance here might start to suffer. Isn't there a rust lib to have a small hash table that does a linear array search below a certain size?
| fn del_fib(&mut self, id: FibKey) { | ||
| info!("Unregistering Fib with id {id} from the FibTable"); | ||
| self.entries.remove(&id); | ||
| self.entries.retain(|_, entry| entry.id != id); |
There was a problem hiding this comment.
So this is a big change, are you certain that there is no place where we depend on just removing the thing indexed by id, but instead want to remove all things with a particular id?
| @@ -168,7 +175,12 @@ fn fmt_nhop_instruction(f: &mut std::fmt::Formatter<'_>, rc: &Nhop) -> std::fmt: | |||
|
|
|||
| // formats nhop using the display of the key, recoursing over resolvers | |||
There was a problem hiding this comment.
Not relevant to the PR, but recoursing isn't the right verb here, recurring is the gerund form for the adjective recursive since they both come from the verb recur.
| Heading(format!("Next-hop Store ({})", self.len())).fmt(f)?; | ||
| for nhop in self.iter() { | ||
| fmt_nhop_rec(f, nhop, 0)?; | ||
| fmt_nhop_rec(f, nhop, 0, &mut Visited::new())?; |
There was a problem hiding this comment.
This seems bad, we are mutating the NhopStore on Display, which is a bad pattern to begin with. Moreover, it seems we never clear this so two next hops that refer to a common nexthop (no loop) would see the visited flag and show as a loop.
There was a problem hiding this comment.
I looked at the tests and I'm not sure it would catch this case either.
There was a problem hiding this comment.
Finally, I don't see anywhere that Visited is ever cleared. Is the NextHop store local to the display logic, or somehow ephemeral?
| error!("Unable to find default VRF!"); | ||
| return RpcResultCode::Failure; | ||
| }; | ||
| vrf0.add_route_rpc(self, None, rmac_store, iftabler); |
There was a problem hiding this comment.
Are there reasons we want the interface name? It seems to me that you could get two next hops that are identical apart from the interface name and they might have two different priorities. Why is this change ok?
No description provided.